Skip to content

[SPARK-6350][Mesos] Fine-grained mode scheduler respects mesosExecutor.cores#8653

Closed
dragos wants to merge 1 commit into
apache:masterfrom
dragos:issue/mesos/fine-grained-maxExecutorCores
Closed

[SPARK-6350][Mesos] Fine-grained mode scheduler respects mesosExecutor.cores#8653
dragos wants to merge 1 commit into
apache:masterfrom
dragos:issue/mesos/fine-grained-maxExecutorCores

Conversation

@dragos

@dragos dragos commented Sep 8, 2015

Copy link
Copy Markdown
Contributor

This is a regression introduced in #4960, this commit fixes it and adds a test.

@tnachen @andrewor14 please review, this should be an easy one.

@dragos

dragos commented Sep 8, 2015

Copy link
Copy Markdown
Contributor Author

I'm working on another fix for SPARK-7874, and will remove some of the duplication in tests, but I wanted to keep this PR as simple as possible (in line with the trivial fix).

@SparkQA

SparkQA commented Sep 8, 2015

Copy link
Copy Markdown

Test build #42132 has finished for PR 8653 at commit 5cd9343.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dragos

dragos commented Sep 9, 2015

Copy link
Copy Markdown
Contributor Author

/cc @skyluc, @nraychaudhuri

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would prefer we use the same matching logic (equals) just to be consistent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean in the assertion below? Not sure what you mean (I think === offers a more detailed error message, but I am ok changing it to == if that's what you mean).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just mean _.getName.equals("cpus") :) Not a big deal, just like to be consistent

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. I can do that. Do you know why equals is preferred? == is safe w.r.t. to nulls, so we assume getName (and all other getters, like getContainerPath) never return null?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's really preferred, it's just how it's written in the first place. I suggest if you think it makes sense either keep it for now or change it everywhere else.

@tnachen

tnachen commented Sep 9, 2015

Copy link
Copy Markdown
Contributor

Good catch! Indeed a regression... this PR LGTM @andrewor14

spark.mesos.mesosExecutor.cores when launching Mesos executors (regression)
@dragos dragos force-pushed the issue/mesos/fine-grained-maxExecutorCores branch from 5cd9343 to 03e8d0a Compare September 9, 2015 08:51
@SparkQA

SparkQA commented Sep 9, 2015

Copy link
Copy Markdown

Test build #42197 has finished for PR 8653 at commit 03e8d0a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dragos

dragos commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

Any committer wants to have a final look? It's a one-liner, but #8671 depends on this (currently I have an ignored test in there, pending this fix). /cc @andrewor14 (@srowen too, in case you have bandwidth..)

@andrewor14

Copy link
Copy Markdown
Contributor

LGTM merging into master 1.5 thanks @dragos.

@asfgit asfgit closed this in f0562e8 Sep 10, 2015
asfgit pushed a commit that referenced this pull request Sep 10, 2015
…or.cores

This is a regression introduced in #4960, this commit fixes it and adds a test.

tnachen andrewor14 please review, this should be an easy one.

Author: Iulian Dragos <jaguarul@gmail.com>

Closes #8653 from dragos/issue/mesos/fine-grained-maxExecutorCores.

(cherry picked from commit f0562e8)
Signed-off-by: Andrew Or <andrew@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants